Security: Sanitize Lightspeed Core error responses#3296
Security: Sanitize Lightspeed Core error responses#3296rajin-kichannagari wants to merge 3 commits into
Conversation
Fixes information disclosure vulnerability where LCS error details were being forwarded directly to clients. Changes: - Add sanitizeLcsError() function to sanitize error responses - Update 4 endpoints to use sanitization - Enhance tests to verify internal details are not exposed
rajin-kichannagari
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
Changed Packages
|
Jdubrick
left a comment
Jdubrick
left a comment
There was a problem hiding this comment.
1 small comment, rest looks good
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3296 +/- ##
==========================================
+ Coverage 53.57% 53.58% +0.01%
==========================================
Files 2407 2409 +2
Lines 86535 86543 +8
Branches 23943 23947 +4
==========================================
+ Hits 46361 46378 +17
+ Misses 38692 38683 -9
Partials 1482 1482
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
JslYoon
left a comment
JslYoon
left a comment
There was a problem hiding this comment.
Overall looks good, although the sanitizeLcsError() function uses any for both errorBody and logger
parameters. We should add proper TypeScript types:
- logger should use the LoggerService type (already imported in types.ts from
@backstage/backend-plugin-api). - errorBody should have an interface that matches the LCS error response
structure (see the test mocks for examples of the shape). - for the function sanitizeLcsError lets capitalize the LCS bit.
If you can also address the SonarCloud analysis and failing CI after rebasing, I can take another look.
- Move sanitizeLCSError to utils.ts for reusability - Fix TypeScript types (use LoggerService, add LCSErrorResponse interface) - Capitalize LCS in function name - Fix failing test for conversation not found - Add unit tests for utils to improve coverage
Extract duplicated error handling pattern into a reusable helper function. This reduces duplication from 5.6% to under 3%.
|
| context: string, | ||
| response: any, | ||
| ): Promise<void> { | ||
| const errorBody = await fetchResponse.json(); |
There was a problem hiding this comment.
Let's wrap this in a try/catch just on the off chance Lightspeed Core doesn't return JSON / doesn't have a body, on the catch we can just set the body to an empty object
There was a problem hiding this comment.
Then let's add a test for this case as well
| fetchResponse: Response, | ||
| logger: LoggerService, | ||
| context: string, | ||
| response: any, |
There was a problem hiding this comment.
Is it possible to type this to express.Response or is ts being weird about it and that is why it's any?
3 participants
Hey, I just made a Pull Request!
Fixes an information disclosure vulnerability where error responses from Lightspeed Core were leaking internal details to clients.
The issue:
When LCS returns errors, we were forwarding the full error message to clients, which could include model names, provider info, org IDs, stack traces, etc. Not great for security.
What I changed:
sanitizeLcsError()function that logs the full error server-side but only returns a generic message to the client/v1/feedback,/v1/query/interrupt,/v1/query, and/v2/conversations/:idExample:
Before:
{"error": "Error from lightspeed-core server: Model gpt-4-0613 failed with OpenAI API error: rate limit exceeded for organization org-abc123"}After:
{"error": "Error from lightspeed-core server while processing query"}Full error details are still logged on the server for debugging.
✔️ Checklist